@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balázs Benics (steakhal)

diff --git a/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp b/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
index 5b5f9df9cb0dc..a7300b7183590 100644
--- a/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
@@ -117,7 +117,12 @@ class CacheInitializer : public DynamicRecursiveASTVisitor {
     }
   }
 
-  CacheInitializer(Ranges &R) : Result(R) {}
+  CacheInitializer(Ranges &R) : Result(R) {
+    ShouldVisitTemplateInstantiations = true;
+    ShouldWalkTypesOfTypeLocs = false;
+    ShouldVisitImplicitCode = false;
+    ShouldVisitLambdaBody = true;
+  }
   Ranges &Result;
 };
 
diff --git a/clang/test/Analysis/suppression-attr.cpp b/clang/test/Analysis/suppression-attr.cpp
index 89bc3c47dbd51..9ba56d976fddb 100644
--- a/clang/test/Analysis/suppression-attr.cpp
+++ b/clang/test/Analysis/suppression-attr.cpp
@@ -1,4 +1,27 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_warnIfReached();
+
+struct Clazz {
+  template <typename T>
+  static void templated_memfn();
+};
+
+// This must come before the 'templated_memfn' is defined!
+static void instantiate() {
+  Clazz::templated_memfn<int>();
+}
+
+template <typename T>
+void Clazz::templated_memfn() {
+  // When we report a bug in a function, we traverse the lexical decl context
+  // of it while looking for suppression attributes to record what source
+  // ranges should the suppression apply to.
+  // In the past, that traversal didn't follow template instantiations, only
+  // primary templates.
+  [[clang::suppress]] clang_analyzer_warnIfReached(); // no-warning
+
+}
 
 namespace [[clang::suppress]]
 suppressed_namespace {

🐧 Linux x64 Test Results

• 111350 tests passed
• 4431 tests skipped

Thank you for the review @ziqingluo-90. I'll adjust the commit message to reflect on your question before landing the change.

EDIT: enhanced the PR summary. I plan to merge the PR tomorrow unless someone objects.

I'm somewhat surprised that it's necessary to visit template instantiations. This machine simply memorizes source locations. Template instantiations have the same source locations as the template itself right? (...right? 😳)

LGTM because I'm sure there's a perfectly reasonable explanation for that.

I see what you mean. I'll have a look tomorrow.
Thanks for the reviews!

I'm somewhat surprised that it's necessary to visit template instantiations. This machine simply memorizes source locations. Template instantiations have the same source locations as the template itself right? (...right? 😳)
LGTM because I'm sure there's a perfectly reasonable explanation for that.

I see what you mean. I'll have a look tomorrow. Thanks for the reviews!

It actually didn't take long. Check the AST of the example: https://godbolt.org/z/MsT5jx3nE

Funnily, the primary template is outside of the CXXRecordDecl. Only the template instantiation is under the CXXRecordDecl.
That should explain the traversal. IDK why don't we have a primary template within the class. Maybe because there can only be exactly 1 primary template. And that is "spelled" outside of the class declaration, thus to remain truthful to the sources, then the primary template AST must also live where it's spelled. I think this is the explanation.
Yeah, there are days when I hate C++, if you ask me.

IDK why don't we have a primary template within the class.

Ohhh. Yeah I'm assuming that this is because compiling uninstantiated templates is so difficult that you can't even tell whether that template actually belongs inside the class at this stage of compilation. You only learn that after substituting the parameter.

(I may be completely wrong but this is the usual explanation for these quirks IIRC.)

(There's no way to put different instantiations in different classes through something like

template <typename T>
void T::foo(){}

right? I hope it's just a simplification that the compilation process does simply because it can.)

On the other hand, I think I know why this wasn't a problem before. It wasn't a problem before because most of the time method templates (or methods of class templates) aren't written out of line. This is because you have to keep them in the header anyway because otherwise you won't be able to instantiate them. So might as well write inline.

Which means that there's probably a small opportunity for optimization there. It may be a good idea to include the uninstantiated template in the scan range while continuing to skip all instantiations? Because that's presumably how everything else works anyway. And this way we won't need to scan all the instantiations that we still don't need to scan. And this could be, like, somewhat noticeable for performance, I think.

Which means that there's probably a small opportunity for optimization there. It may be a good idea to include the uninstantiated template in the scan range while continuing to skip all instantiations? Because that's presumably how everything else works anyway. And this way we won't need to scan all the instantiations that we still don't need to scan. And this could be, like, somewhat noticeable for performance, I think.

Then I think explicit template specializations would break. The unfortunate fact is that primary templates are not that useful for us.

To sidestep this, what if we would not overrule the original DeclWithIssue and just traverse that one. That will be the correct fn to traverse.
Besides that, would need to retrofit the lexical scope behavior, which is (if we can trust the comment) about enclosing suppress attributes attached to the parent CXXRecordDecls.
If that was the only motivation there, then its easy to just check exactly that.

But honestly, I just probably wouldnt touch this.

It's actually much weirder than that. Your example only causes problems when the implicit instantiation is requested *above* the definition. And that's also a prerequisite for the quirky AST. Compare: https://godbolt.org/z/4eT8Er3ev

So I think explicit specializations are actually fine because it's an error to perform them after an implicit instantiation: https://godbolt.org/z/ddT4bffvK

Yeah, thanks for looking at this again.
So, are we still good with this patch?

Yeah I think this is good to go.

I think it'd be a better solution to use the information that we've already gathered, instead of mining more information. But that's most likely a non-issue.